Tableflow update patch - #214
Draft
Jared Breeden (jbreeden) wants to merge 3 commits into
Draft
Conversation
… int64) TableflowTopicConfig.to_spec() passed retention_ms/data_retention_ms straight through, so a plain int (the natural way to construct it) hit the wire as a JSON number. Confirmed against the live API: it rejects non-string values for both, since the schema types them as int64, string-encoded to dodge JS/IEEE-754 double precision loss. to_spec() now always stringifies both.
PATCH /tableflow/v1/tableflow-topics/{display_name}, mirroring
enable_tableflow's own structured shape: table_formats/config are
independently optional, None meaning "leave unchanged" (never "clear it" --
retention_ms/data_retention_ms/error_handling all have server-enforced
defaults and the server rejects an explicit null for any of them).
Diffing against live state to decide what's actually changing is the
caller's job, not this driver's -- this just sends whatever it's given, same
as every other Tableflow request-building function here.
Also introduces Fields (wire field names as plain string constants, not an
Enum -- StrEnum needs Python 3.11+, and the older str/Enum mixin returns the
qualified member name from str()/formatting rather than the plain value) and
parses spec.config into a typed TableflowTopicConfig on read (previously left
raw), since update_tableflow's callers need to diff against real typed
state, not a dict.
Copilot started reviewing on behalf of
Jared Breeden (jbreeden)
September 8, 2026 22:51
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Response parsing can currently leak non-DB-API exceptions on malformed numeric fields, and update_tableflow can emit an unhelpful empty error-detail message.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds first-class support for updating existing Tableflow topics (PATCH) and tightens Tableflow config modeling to consistently round-trip the API’s string-encoded int64 fields while dropping deprecated/read-only config keys.
Changes:
- Introduce
Connection.update_tableflow(..., wait_for_running=True, timeout=...)and PATCH payload shaping (build_update_payload). - Model
TableflowTopicSpec.configas a typedTableflowTopicConfigwithfrom_spec()parsing and string-encoding of int64 fields on the wire. - Update/add unit tests to validate request/response shapes and wait-for-RUNNING behavior.
File summaries
| File | Description |
|---|---|
| tests/unit/test_tableflow_unit.py | Updates config expectations (int↔wire-string) and adds from_spec() round-trip tests. |
| tests/unit/test_tableflow_connection_unit.py | Adds unit coverage for update_tableflow request shaping, empty-update guard, error mapping, and wait loop behavior. |
| src/confluent_sql/tableflow.py | Adds Fields constants, typed config parsing/serialization, and PATCH payload builder. |
| src/confluent_sql/connection.py | Adds Connection.update_tableflow implementation with error mapping and optional wait-for-RUNNING. |
| src/confluent_sql/init.py | Exposes TableflowTopicSpec from the package surface. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- update_tableflow's error-detail extraction fell back to "no more details" only via an exception; a parseable body with an empty (or detail-less) errors list silently produced an empty message instead. - TableflowTopic.from_response only wrapped KeyError as OperationalError; optional_int_from_str's int(s) can raise ValueError/TypeError on a malformed wire value, which would otherwise leak past this response- parsing boundary as a raw exception. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Copilot started reviewing on behalf of
Jared Breeden (jbreeden)
September 8, 2026 23:07
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are a few verified edge cases in Tableflow response parsing/defaulting (null target, uncaught AttributeError, and raw-config accessibility) that should be addressed to avoid breaking callers and leaking inconsistent exceptions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/confluent_sql/tableflow.py:266
error_handling_from_spec()usesdata.get(..., "error_log"), which will returnNone(not the default) if the key exists but is null on the wire, producingTableflowErrorHandlingLog(target=None)and then emitting"target": Noneon round-trip. Prefer treating null/empty as missing so the default is always a real string.
src/confluent_sql/tableflow.py:439TableflowTopicSpec.configis now typed (TableflowTopicConfig) and drops unmodeled wire keys (e.g., deprecated/read-only flags). That’s fine internally, but it’s a behavioral change for callers that previously inspected those raw config fields; consider providing a small compatibility/accessor to the raw wire config dict to keep those values discoverable without reaching intospec.raw.
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
473
to
+476
| except KeyError as e: | ||
| raise OperationalError(f"Error parsing Tableflow topic response, missing {e}.") from e | ||
| except (ValueError, TypeError) as e: | ||
| raise OperationalError(f"Error parsing Tableflow topic response: {e}") from e |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft for now. Finishing up some manual testing before I write up the full PR description for this & associated dbt-side changes.
TODO: Do we need to pull c6a6dfc into any active release branch?